feat: Set tmpspace with help of gres="tmpspace:10G" syntax#444
feat: Set tmpspace with help of gres="tmpspace:10G" syntax#444cmeesters merged 11 commits intosnakemake:mainfrom
Conversation
Add a test for valid GRES format with tmpspace.
Updated GRES validation to include tmpspace format.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR updates Slurm GRES validation to accept an optional trailing unit letter (T, G, or M) immediately after numeric values in generic resource specs, adjusts error message text accordingly, and adds a unit test verifying a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can disable the changed files summary in the walkthrough.Disable the |
maarten-k
changed the title
maarten-k
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/further.md`:
- Line 383: Remove or correctly format the stray literal "GPU Jobs" line that
currently sits inside the MPI section; locate the orphan string "GPU Jobs" and
either delete it or convert it into the proper heading (e.g., "## GPU Jobs") at
the start of the actual GPU section so the document structure remains consistent
with the existing MPI section content.
- Around line 521-526: Update the "Setting tmpspace with gress" section: fix the
header and body typos by replacing "gress" with "GRES" (use uppercase), "cluser"
with "cluster", and "workernode" with "worker node" so the section reads e.g.
"Setting tmpspace with GRES" and mentions "cluster" and "worker node" in the
explanatory sentence; keep the note about cluster-specific configuration as-is.
In `@snakemake_executor_plugin_slurm/utils.py`:
- Around line 296-305: The WorkflowError message for invalid GRES (variable
gres) is split across adjacent string literals that concatenate without spaces,
causing run-together text; update the raise WorkflowError(...) and the preceding
message construction to include proper spacing between parts—either by adding
explicit spaces at the end/start of the fragmented string literals or by
replacing the fragmented literals with a single formatted string (or
triple-quoted string) that includes the needed spaces so the expected format
text reads correctly (e.g., in the raise WorkflowError(...) and the earlier
error branch where the message is built).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9b6e53f1-c915-4491-b2a3-c3a5ee06d135
📒 Files selected for processing (3)
docs/further.mdsnakemake_executor_plugin_slurm/utils.pytests/tests.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
snakemake_executor_plugin_slurm/utils.py (1)
290-306: Reduce duplicated GRES format strings in error handling.The branch is correct, but the expected-format text is repeated in two places. Hoisting it into one constant will prevent drift and keep future edits safer.
♻️ Suggested cleanup
if job.resources.get("gres"): # Validate GRES format (e.g., "gpu:1", "gpu:tesla:2") gres = job.resources.gres - if not gres_re.match(gres): - if not gres_tmpspace_re.match(gres): - if not string_check.match(gres): - raise WorkflowError( - "GRES format should not be a nested string (start " - "and end with ticks or quotation marks). " - "Expected format: " - "'<name>:<number>' or '<name>:<type>:<number>' or " - "'tmpspace:<number>[TGM]' " - "(e.g., 'gpu:1' or 'gpu:tesla:2') or tmpspace:10G " - ) - else: - raise WorkflowError( - f"Invalid GRES format: {gres}. Expected format: " - "'<name>:<number>' or '<name>:<type>:<number>' " - "or 'tmpspace:<number>[TGM]' " - "(e.g., 'gpu:1' or 'gpu:tesla:2') or tmpspace:10G " - ) + expected_gres_format = ( + "'<name>:<number>' or '<name>:<type>:<number>' or " + "'tmpspace:<number>[TGM]' (e.g., 'gpu:1', 'gpu:tesla:2', or 'tmpspace:10G')" + ) + if not (gres_re.match(gres) or gres_tmpspace_re.match(gres)): + if not string_check.match(gres): + raise WorkflowError( + "GRES format should not be a nested string (start " + "and end with ticks or quotation marks). " + f"Expected format: {expected_gres_format}" + ) + raise WorkflowError( + f"Invalid GRES format: {gres}. Expected format: {expected_gres_format}" + ) return f" --gres={job.resources.gres}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@snakemake_executor_plugin_slurm/utils.py` around lines 290 - 306, The two WorkflowError branches in the GRES validation duplicate the same expected-format message; extract that message into a single constant (e.g., GRES_EXPECTED_FORMAT) and reference it from both error raises in the function containing gres_tmpspace_re and string_check checks so both branches call WorkflowError(GRES_EXPECTED_FORMAT) or use f"Invalid GRES format: {gres}. {GRES_EXPECTED_FORMAT}" where needed; update imports/locals as necessary and ensure the constant name is unique and placed near the top of the module for reuse.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@snakemake_executor_plugin_slurm/utils.py`:
- Around line 290-306: The two WorkflowError branches in the GRES validation
duplicate the same expected-format message; extract that message into a single
constant (e.g., GRES_EXPECTED_FORMAT) and reference it from both error raises in
the function containing gres_tmpspace_re and string_check checks so both
branches call WorkflowError(GRES_EXPECTED_FORMAT) or use f"Invalid GRES format:
{gres}. {GRES_EXPECTED_FORMAT}" where needed; update imports/locals as necessary
and ensure the constant name is unique and placed near the top of the module for
reuse.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d2b013d2-1b52-458a-a4cb-e8d70f986068
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/utils.py
Corrected the heading format for GRES and updated the text for clarity.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/further.md (1)
524-524:⚠️ Potential issue | 🟡 MinorTypo: "cluser" should be "cluster".
The word "cluser" is misspelled. Additionally, the note would benefit from proper capitalization and punctuation for consistency with the rest of the documentation.
📝 Suggested correction
-*this depends on the cluser config and is not a default* +*This depends on the cluster configuration and is not a default.*🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/further.md` at line 524, Fix the typo and style in the italic note "*this depends on the cluser config and is not a default*" by replacing "cluser" with "cluster", capitalizing the first word ("This") and adding terminal punctuation (e.g., a period) so the sentence reads: "This depends on the cluster config and is not a default." Ensure the exact italic formatting is preserved around the corrected sentence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/further.md`:
- Line 524: Fix the typo and style in the italic note "*this depends on the
cluser config and is not a default*" by replacing "cluser" with "cluster",
capitalizing the first word ("This") and adding terminal punctuation (e.g., a
period) so the sentence reads: "This depends on the cluster config and is not a
default." Ensure the exact italic formatting is preserved around the corrected
sentence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d6dea984-5380-44a9-891b-c24de96cab01
📒 Files selected for processing (1)
docs/further.md
|
Hi Maarten, Thank you for your contribution! Currently, we set the As for the documentation: I very much prefer not to clutter it with exceptions for certain clusters. We recently introduced https://github.com/snakemake/snakemake-cluster-profiles/ where you can contribute a cluster profile including annotation for certain additional flags in a readme file. Let me know what you think. |
|
I think that changing the current regex to: |
Removed outdated section on setting tmpspace with GRES and clarified resource specifications for MPI jobs.
Updated GRES regex to include optional postfix for units.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@snakemake_executor_plugin_slurm/utils.py`:
- Around line 286-287: The if statement calling gres_re.match is missing a
closing parenthesis and colon which causes a syntax error; edit the conditional
around gres_re.match(gres) so it reads "if not gres_re.match(gres):" (and keep
or adjust the subsequent "if not string_check.match(gres):" as intended),
ensuring proper indentation and that each if has its closing ":" to restore
valid syntax in the function using gres_re.match and string_check.match.
- Around line 298-299: The error message has two adjacent string literals
concatenated without a space, causing "...postfix(e.g.,..." to be printed;
locate the concatenation in utils.py where the message pieces ending with
"postfix" and starting with "(e.g., 'gpu:1'..." are joined (the string used for
the value/error text), and insert a space either at the end of the first literal
or at the start of the second so the output becomes "...postfix (e.g.,...". Keep
the same string literals and formatting otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b218753-fbdb-4cd1-b928-1726c9fbf445
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/utils.py
lines where to long
cmeesters
left a comment
cmeesters
left a comment
There was a problem hiding this comment.
As discussed: good to go. Will be released asap.
🤖 I have created a release *beep* *boop* --- ## [2.6.0](v2.5.4...v2.6.0) (2026-03-26) ### Features * job arrays ([#174](#174)) ([a51a5bf](a51a5bf)) * Set tmpspace with help of gres="tmpspace:10G" syntax ([#444](#444)) ([c6f6658](c6f6658)) ### Bug Fixes * logo ([#439](#439)) ([c4e3ec3](c4e3ec3)) ### Documentation * add admin documentation ([#436](#436)) ([8f7491f](8f7491f)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for job arrays * Added tmpspace configuration capability via `gres="tmpspace:10G"` * **Bug Fixes** * Fixed logo display issue * **Documentation** * Added admin documentation <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Set tmpspace with help of gres="tmpspace:10G" syntax
This is probably useful for only a few clusters [1], so the regex for the tmpspace is limited to tmpspace only.
[1] at least on "UMC utrecht HPC" system
Summary by CodeRabbit
New Features
Tests